fix(daemon): write daemon status via sync write_atomic (#865)#884
Conversation
`StatusManager::write_atomic` bridged to the async `fbuild_core::fs::write_atomic` via `tokio::task::block_in_place` + `Handle::block_on`. `block_in_place` panics inside a current-thread tokio runtime — which is the default flavor of `#[tokio::test]` — so every unit test that constructed a `DaemonContext` (which constructs a `StatusManager`, whose `new` calls `flush()`, which calls `write_atomic`) panicked on macOS + Windows CI. Three named tests are affected: - handlers::health::tests::shutdown_refuses_non_force_when_operation_in_progress - handlers::locks::tests::lock_status_reports_held_project_locks_without_stale_entries - handlers::locks::tests::clear_locks_removes_only_unheld_project_lock_entries Fix: add `fbuild_core::fs::write_atomic_sync` — same write-tempfile / fsync / rename semantics as the async helper, but `std::fs` end-to-end so it has no runtime-flavor requirement. Switch `StatusManager::write_atomic` to call it. The status file's atomicity contract is preserved verbatim; the only behavior change is that the call is no longer runtime-aware. Two new regression tests in `fs::tests`: - `write_atomic_sync_runs_inside_current_thread_runtime` (the exact scenario that was panicking) - `write_atomic_sync_works_without_runtime` (cold-bootstrap path) Closes #865
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Sync atomic write + StatusManager migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Rolls up the fixes shipped since 2.3.14: - #855 (#883) — drop stale standalone-zccache CI/Docker/docs references after the soldr embedded-zccache transition (soldr#977/#980/#1081) - #865 (#884) — write daemon status via sync write_atomic; unblocks fbuild-daemon unit tests on macOS/Windows by eliminating the block_in_place panic in current-thread tokio runtimes - #875 (#885) — pin TMP/TEMP for compiler subprocess on Windows; ESP32 compile no longer fails with 'Cannot create temporary file in C:\Windows\' on the very first TU - #720 (#886) — record anti-removal policy for dump_usb_ids example - #829 (#887) — stage fbuild._native cdylib during source/editable install so 'from fbuild import ...' works after 'uv pip install -e .' Plus all prior #664 platform_packages audit work, the #826 testing followups, and the build cancellation fix from earlier in the cycle.
Problem
StatusManager::write_atomicbridged to the asyncfbuild_core::fs::write_atomicviatokio::task::block_in_place+Handle::block_on.block_in_placepanics inside a current-thread tokio runtime — the default flavor of#[tokio::test]— so every unit test that constructed aDaemonContext(which constructs aStatusManager, whosenewcallsflush(), which callswrite_atomic) panicked on macOS + Windows CI. Three named tests were affected:handlers::health::tests::shutdown_refuses_non_force_when_operation_in_progresshandlers::locks::tests::lock_status_reports_held_project_locks_without_stale_entrieshandlers::locks::tests::clear_locks_removes_only_unheld_project_lock_entriesFix
Add
fbuild_core::fs::write_atomic_sync— same write-tempfile / fsync / rename semantics as the async helper, butstd::fsend-to-end so it has no runtime-flavor requirement. SwitchStatusManager::write_atomicto call it. The status file's atomicity contract is preserved verbatim; the only behavior change is that the call is no longer runtime-aware.Why sync instead of runtime-flavor sniffing
The fix sketch in the issue proposed
Handle::current().runtime_flavor()matching to route current-thread runtimes to a spawnedtokio::runtime::Runtimefor theblock_on. That nests runtimes (also disallowed) and adds a per-writeRuntime::new()allocation. A syncstd::fshelper sidesteps both issues — and the status writer was already inside synchronous code paths inside a sync-lockedMutex<DaemonStatus>, so there's no async-blocking concern.Tests
Two regression tests in
fs::tests:write_atomic_sync_runs_inside_current_thread_runtime— exact panic scenario, asserts no panic + correct contentwrite_atomic_sync_works_without_runtime— cold-bootstrap pathVerification
soldr cargo check -p fbuild-core -p fbuild-daemoncleansoldr cargo clippy -p fbuild-core -p fbuild-daemon --all-targets -- -D warningscleansoldr cargo test -p fbuild-core --lib fs::tests::write_atomic_sync_cleanCloses #865
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes